DataStore 토큰 저장 기능 구현 & Ktor Token 로직 설정#59
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthrough프로젝트에 두 개의 새로운 모듈이 추가되었습니다: Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@app/build.gradle.kts`:
- Around line 33-34: Remove the unnecessary direct dependency declaration
implementation(projects.core.token) from the app module's build script because
app does not directly import the token package and core.datastore already
depends on core.token transitively; update the dependencies block to omit
implementation(projects.core.token), run a local build to confirm no missing
symbols, and if the build fails, re-add only after verifying which classes
require core.token (check for any direct imports of the token package or usages
of TokenProvider/related types).
In `@core/datastore/src/main/java/com/twix/datastore/AuthConfigure.kt`:
- Around line 13-21: AuthConfigure currently hardcodes realistic-looking JWTs in
the accessToken and refreshToken defaults; change those defaults to empty
strings and ensure any code constructing or persisting AuthConfigure supplies
real tokens explicitly. Locate the AuthConfigure data class and replace the
default values for the properties accessToken and refreshToken with "" (empty
string), and audit any callers that rely on the defaults (constructors or
deserialization paths) so they fail-fast or validate tokens are present before
making network calls.
- Around line 28-35: In readFrom (the suspend function that decodes with
AuthConfigure.serializer()), don’t swallow SerializationException and return
defaultValue; instead catch SerializationException and rethrow it wrapped in an
androidx.datastore.core.CorruptionException (including the original exception as
cause and a clear message) so DataStore's CorruptionHandler can run and handle
recovery/logging; update imports to include CorruptionException and remove the
fallback return defaultValue in that catch.
In `@core/datastore/src/main/java/com/twix/datastore/AuthTokenProvider.kt`:
- Around line 3-17: AuthTokenProvider currently calls dataStore.data.first() and
dataStore.updateData() directly in methods accessToken(), refreshToken(),
saveToken(), and clear(), which can throw IO/serialization exceptions; wrap
these calls to catch exceptions and return safe defaults or no-ops: use a
try-catch around dataStore.data.first() in accessToken()/refreshToken() to
return empty string (or configured default) on failure, and wrap
dataStore.updateData() in saveToken()/clear() to catch exceptions and log/ignore
failures; alternatively apply Flow.catch on dataStore.data before first() to
handle errors; ensure any SerializationException from AuthConfigureSerializer is
still covered and add brief logging where appropriate.
In `@core/datastore/src/main/java/com/twix/datastore/DataStore.kt`:
- Around line 7-10: The current Context.authDataStore (storing AuthConfigure via
AuthConfigureSerializer to "auth-configure.json") persists
accessToken/refreshToken in plaintext; replace this with an encrypted storage
solution: either migrate to EncryptedSharedPreferences
(androidx.security:security-crypto) and persist AuthConfigure fields securely or
adapt DataStore to use an encryption layer (e.g., encrypt/decrypt the serialized
bytes with Jetpack Security or Tink before writing/after reading) so tokens are
never stored as plaintext; update usages that read/write Context.authDataStore
to use the new encrypted provider and ensure the serializer logic
(AuthConfigureSerializer) is adjusted to operate on encrypted payloads or plain
objects as appropriate.
🧹 Nitpick comments (2)
core/datastore/proguard-rules.pro (1)
1-21: Kotlin Serialization을 위한 ProGuard 규칙 추가 검토현재 파일은 기본 템플릿만 포함되어 있습니다.
datastore모듈에서@Serializable어노테이션을 사용하는AuthConfigure클래스가 있다면, R8/ProGuard 최적화 시 직렬화가 정상 동작하도록 추가 규칙이 필요할 수 있습니다.Kotlin Serialization 라이브러리 자체에 consumer ProGuard 규칙이 포함되어 있어 대부분의 경우 자동으로 처리되지만, release 빌드에서 문제가 발생한다면 다음과 같은 규칙 추가를 고려해 주세요:
# Keep `@Serializable` classes -keepattributes *Annotation*, InnerClasses -keepclassmembers `@kotlinx.serialization.Serializable` class ** { *; }core/datastore/src/main/java/com/twix/datastore/di/DataStoreModule.kt (1)
7-10: 명시적 Application Context 사용으로 의도 명확히 하기현재 코드는
initKoin()호출 시 Application 인스턴스가androidContext()로 등록되므로 실제로는 Application Context가 주입됩니다. 하지만get()만으로는 어떤 Context가 주입되는지 명확하지 않습니다. DataStore는 Application Context를 사용해야 안전하므로, 의도를 명시적으로 표현하기 위해androidContext()사용을 권장합니다.🔧 수정 예시
+import org.koin.android.ext.koin.androidContext import org.koin.dsl.module val dataStoreModule = module { - single<TokenProvider> { AuthTokenProvider(get()) } + single<TokenProvider> { AuthTokenProvider(androidContext()) } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@data/build.gradle.kts`:
- Around line 9-11: Remove the unused core.datastore dependency from the data
module's dependencies block by deleting the
implementation(projects.core.datastore) entry; verify
DefaultOnboardingRepository (which only depends on
com.twix.network.service.OnboardingService and domain types) does not reference
core.datastore, and if token functionality is needed refactor the repository to
depend on the core.token module and the TokenProvider interface (inject
TokenProvider into DefaultOnboardingRepository) to follow dependency inversion.
🧹 Nitpick comments (1)
core/network/src/main/java/com/twix/network/service/AuthService.kt (1)
8-13: AuthService 인터페이스가 명확하게 정의되었습니다.Ktorfit 규약을 잘 따르고 있으며, suspend 함수로 코루틴을 지원합니다.
⚠️ 향후 토큰 인증 구현 시 고려사항:현재
HttpClientProvider.kt에서 Auth 플러그인이 주석 처리된 상태입니다(30번째 줄의 TODO 참고). Auth 플러그인을 활성화할 때,refresh엔드포인트는 다음과 같이 처리해야 합니다:
- Authorization 헤더 자동 추가 제외 - refresh 호출 시 만료된 AccessToken 헤더가 자동으로 포함되면 401 에러 발생 가능
- 무한 루프 방지 - 401 응답 시 자동으로 refresh를 시도하는 로직이 있다면, refresh 엔드포인트 자체는 이 로직에서 제외되어야 함
Auth 플러그인 구현 시 이 엔드포인트를 특별하게 처리하는 방안을 미리 설계하는 것을 권장합니다. 예를 들어 refresh 요청을 감지하는 로직을 추가하거나, RefreshRequest를 구분하는 마커를 사용할 수 있습니다.
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
고생하셨습니다! IO 디스패처랑 토큰 캐시는 챙기면 좋을 것 같아요
There was a problem hiding this comment.
Json 인스턴스를 매번 생성하지 말고 미리 선언해놓고 사용하면 좋을 거 같아요! 보통 Json 미리 만들어 둘 때
private val json = Json {
ignoreUnknownKeys = true
}
이렇게 모르는 키는 무시해서 Exception 발생 안하게 하기도 해요. 추가 설정 필요한 거 있으면 더 추가해도 됩니다!
There was a problem hiding this comment.
좋은 생각인거 같아 !
리뷰 반영 커밋 : https://github.com/YAPP-Github/Twix-
Android/pull/59/commits/773011e5c9b140d39d62bdeb77f9e6d436d30c69
| override suspend fun readFrom(input: InputStream): AuthConfigure = | ||
| try { | ||
| Json.decodeFromString( | ||
| deserializer = AuthConfigure.serializer(), | ||
| string = input.readBytes().decodeToString(), | ||
| ) | ||
| } catch (e: SerializationException) { | ||
| defaultValue | ||
| } |
There was a problem hiding this comment.
여기에서는 IO 디스패처를 사용하지 않는 이유가 있나요?
There was a problem hiding this comment.
앗.. 단순 실수 ^_^... 꼼꼼하게 봐줘서 고마워 !
리뷰 반영 커밋 : 31b2b9b
There was a problem hiding this comment.
토큰을 매번 처음부터 읽지 않고 StateIn 사용해서 StateFlow로 캐싱하면 더 좋을 거 같아요! stateIn 쓰려면 스코프가 있어야 하는데 Koin으로 주입받거나 내부에서 SupervisorJob 기반으로 스코프 만들어서 쓰면 될 것 같습니다.
There was a problem hiding this comment.
@Serializable
internal data class AuthTokens(
val accessToken: String = "",
val refreshToken: String = "",
)
이미 만들어두신 AuthConfigure 사용해도 되고 이런 거 추가해서 캐싱할 때 access, refresh 둘 다 들고 있으면 두번 읽지 않게 구현할 수 있을 것 같아요
- `TokenProvider`의 `accessToken`과 `refreshToken`을 suspend 함수에서 `val` 프로퍼티로 변경하여 동기적으로 토큰에 접근할 수 있도록 수정 - `AuthTokenProvider`에서 DataStore의 데이터를 `stateIn`을 사용해 `StateFlow`로 관찰하고, 토큰 값을 캐싱하여 즉시 반환하도록 로직 개선 - 변경된 `AuthTokenProvider` 생성자에 `CoroutineScope` 의존성 추가
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/datastore/src/main/java/com/twix/datastore/AuthConfigure.kt`:
- Around line 12-22: The AuthConfigure data class currently hardcodes
real-looking JWTs in the accessToken and refreshToken default values; change
both default initializers in AuthConfigure (accessToken and refreshToken) to
empty strings ("") and update any code that constructs or uses AuthConfigure
(e.g., places that instantiate AuthConfigure or call methods relying on
non-empty tokens) to explicitly handle the empty-token case (validate presence,
throw clear errors, or skip token-based calls) so no hardcoded credentials are
persisted or assumed.
🧹 Nitpick comments (3)
core/datastore/src/main/java/com/twix/datastore/AuthConfigure.kt (2)
34-44:withContext(Dispatchers.IO)래핑이 중복될 수 있습니다.DataStore는 내부적으로 이미 I/O 작업을 적절한 디스패처에서 처리합니다.
readFrom과writeTo에서 추가로withContext(Dispatchers.IO)를 사용하는 것은 불필요한 컨텍스트 스위칭을 유발할 수 있습니다.다만, 명시적으로 I/O 디스패처를 지정하는 것이 팀의 컨벤션이라면 유지해도 무방합니다. 이 부분은 팀과 논의 후 결정하시면 좋겠습니다.
42-43: IOException도 함께 처리하면 더 안전합니다.현재
SerializationException만 catch하고 있어서, 파일 I/O 오류(손상된 파일, 권한 문제 등) 발생 시 예외가 전파됩니다.♻️ 예외 처리 개선 제안
override suspend fun readFrom(input: InputStream): AuthConfigure = try { withContext(Dispatchers.IO) { json.decodeFromString( deserializer = AuthConfigure.serializer(), string = input.readBytes().decodeToString(), ) } - } catch (e: SerializationException) { + } catch (e: Exception) { + // SerializationException, IOException 등 모든 예외에서 기본값 반환 defaultValue }app/src/main/java/com/yapp/twix/di/InitKoin.kt (1)
25-26: 모듈 등록 순서를 명확히 하면 좋겠습니다.
dataStoreModule은CoroutineScope에 의존하고, 이는appModule에서 제공됩니다. Koin은 지연 해석(lazy resolution)을 하므로 현재 순서로도 동작하지만, 의존성 방향을 명확히 하기 위해appModule을 먼저 등록하는 것이 가독성에 좋습니다.♻️ 순서 조정 제안
add(uiModule) - add(dataStoreModule) add(appModule) + add(dataStoreModule)
chanho0908
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
이슈 번호
#57
리뷰/머지 희망 기한 (선택)
작업내용
결과물
리뷰어에게 추가로 요구하는 사항 (선택)
같은 core 레벨에 있는 network 모듈과 datastore이 서로 직접 의존하지 않도록 core:token 모듈을 추가하고
인터페이스를 구현해 의존성을 역전시켰어 ! network --> token(interface) <-- datastore (concrete)